Fix nested lambda inference memory growth #5206 - #5207
Conversation
871baa6 to
742245e
Compare
742245e to
7b1dd77
Compare
martinfrancois
changed the title
Link nested speculative copies to their canonical source lambdas so parameterless lambda chains reuse existing per-target inference results without sharing parameter bindings or parser state. Fixes eclipse-jdt#5206 Signed-off-by: François Martin <f.martin@fastmail.com>
7b1dd77 to
97e0045
Compare
There was a problem hiding this comment.
Pull request overview
This PR addresses a heap-exhaustion scenario in ECJ speculative overload resolution involving deeply nested lambdas by ensuring nested speculative lambda copies remain linked to their canonical source lambdas and can reuse per-target inference caches when safe (no parameters in the enclosing lambda chain). It also adds a focused regression suite to prevent reintroducing the combinatorial memory growth.
Changes:
- Update
LambdaExpression.copy()to align source and copied nested lambdas, linking eligible nested copies to canonical source lambdas and sharingcopiesPerTargetTypecaches when the enclosing lambda chain is parameterless. - Add a new regression test (
NestedLambdaInferenceTest) that generates deep nested overload/lambda chains (generic + concrete variants). - Register the new regression test in the compiler regression test suite.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/TestAll.java | Adds the new regression test class to the standard test suite. |
| org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/NestedLambdaInferenceTest.java | New regression coverage for nested speculative overload resolution without heap blowups. |
| org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/LambdaExpression.java | Links nested speculative lambda copies to canonical sources and shares inference caches where safe. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@stephan-herrmann - given this involves inference, I am assuming you will want to be the first reviewer, I have also signed up to review FYI |
stephan-herrmann
left a comment
stephan-herrmann
left a comment
There was a problem hiding this comment.
Some questions after a first scan of changes:
- could we have some tests that demonstrate the interleaving of lambdas with and without parameters? There is some complexity in this, which I'd like see live in action.
- would it make sense to add a shortcut such that "normal" scenarios could continue to work the "old" way?
Replace the traversal-index root check with an identity check, document context-local cache ownership below parameterized lambdas, and cover interleaved parameterized and parameterless lambdas. Signed-off-by: François Martin <f.martin@fastmail.com>
stephan-herrmann
left a comment
stephan-herrmann
left a comment
There was a problem hiding this comment.
I'm starting to understand the strategy 😄
I suggest to start the next round by looking at my comment in method collectLambdas(). If my idea there is sound, then some of the other comments might become obsolete.
|
Thanks, your pruning idea works and made the code much simpler.
I added the details in the inline replies. After this change, all 54 I also updated the pull request description to match the new approach. |
Collect a lambda with parameters and then stop below it. Nested lambdas may use that lambda's parameter bindings, so their caches must stay local to the enclosing copy. The lambda itself can share its cache because each cached copy has its own parameter bindings. Remove the cache eligibility record and depth counter. Let shareInferenceCaches() create copiesPerTargetType in one place. Signed-off-by: François Martin <f.martin@fastmail.com>
dd2de41 to
b25d5f0
Compare
Thanks for making these changes. The PR now looks good to me. @srikanth-sankaran do you want review this, too? |
What it does
Fixes #5206.
During speculative overload resolution, ECJ tries each overload candidate without committing to one. It reparses the outer lambda to create a separate copy for that candidate. Each source lambda stores inference results for the target functional-interface types against which ECJ has already tested it. The lambda from which a copy was made is its original lambda.
The outer speculative copy kept that source link, but nested lambdas inside the copy were treated as new originals. They therefore did not reuse the existing per-target results from their original lambdas. Repeating inference for every candidate at every nested level caused combinatorial memory growth and exhausted the heap.
The change links nested speculative copies to their original lambdas. On each branch, it shares the per-target inference cache down to and including the first lambda with parameters. A cache entry is a resolved copy of that lambda and owns its parameter bindings. Nested lambdas may use those bindings, so traversal stops below it and their caches stay local to the enclosing copy. No global cache or shared parser state is introduced.
The source lambda and the copy created by parsing the source again are traversed in the same source order. Each traversal records the current lambda, then stops below it when it has parameters.
shareInferenceCaches()therefore receives only lambdas allowed by this parameter-binding rule, so no separate eligibility flag is needed.The independently designed regression suite has three tests. Two generate a generic and a concrete source. Each source contains 24 nested method calls and 23 parameterless lambdas, alternates between
routeandselect, provides five overload candidates for each selector, and ends at aWork<String>field. The two variants exercise the original missing cache reuse with different inference pressure.The third test compiles and runs code that mixes lambdas with and without parameters. It includes a nested lambda that reads an enclosing parameter and a separate parameterless sibling. There is no public API change.
The linked NestedLambdaGenerics.java and NestedLambdaNoGenerics.java files provide separate reference validation for the reported behavior; they are not copied into the regression suite.
How to test
792a9156c119d28063904c85726e486ec2fb2206, compiling each linked OpenJDK source separately with-25and a 1 GiB heap reproduces Nested lambda overload inference exhausts the heap #5206.OutOfMemoryErroror internal compiler error.NestedLambdaInferenceTestacross Java 8 through Java 25 compliance levels: 54 tests pass.GenericsRegressionTest_1_8together withNestedLambdaInferenceTest: 5,670 tests pass, including the parameterized-lambda regressiontestBug496578.parser/TestAll,regression/TestAll, andeval/TestAll): 263,722 tests pass with no failures, errors, or skipped tests.Author checklist